-
Notifications
You must be signed in to change notification settings - Fork 14.9k
KAFKA-20110 Fix generated doc and Javadoc for dynamic configs #21384
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
|
is this duplicate to #21131? |
| .define(QuotaConfig.REPLICA_ALTER_LOG_DIRS_IO_MAX_BYTES_PER_SECOND_CONFIG, ConfigDef.Type.LONG, | ||
| QuotaConfig.QUOTA_BYTES_PER_SECOND_DEFAULT, ConfigDef.Range.atLeast(0), | ||
| ConfigDef.Importance.MEDIUM, QuotaConfig.REPLICA_ALTER_LOG_DIRS_IO_MAX_BYTES_PER_SECOND_DOC); | ||
| return BROKER_QUOTA_CONFIG_DEF; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new ConfigDef is required for each call because the one from brokerQuotaConfigs is subject to modification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think problem is more deeper ... In the, we can see
kafka/server/src/main/java/org/apache/kafka/server/config/DynamicConfig.java
Lines 36 to 43 in 9d5bbf5
| ConfigDef configs = QuotaConfig.brokerQuotaConfigs(); | |
| // Filter and define all dynamic configurations | |
| AbstractKafkaConfig.CONFIG_DEF.configKeys().forEach((name, value) -> { | |
| if (DynamicBrokerConfig.ALL_DYNAMIC_CONFIGS.contains(name)) { | |
| configs.define(value); | |
| } | |
| }); | |
| BROKER_CONFIGS = configs; |
I don't why there is QuotaConfig.brokerQuotaConfigs(); as other configs is loaded from ALL_DYNAMIC_CONFIGS. Maybe that's because someone forgot to include it inside the ALL_DYNAMIC_CONFIGS backthen...
Anyway, I have removed/replaced ConfigDef configs = QuotaConfig.brokerQuotaConfigs() as it is already included and defined in the ALL_DYNAMIC_CONFIGS, and it seems to work. Also checked the website and showing the right values.
Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe that's because someone forgot to include it inside the ALL_DYNAMIC_CONFIGS backthen...
yes, that's indeed an issue, and #21131 tries to fix it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but still this line of code.
| ConfigDef configs = QuotaConfig.brokerQuotaConfigs(); |
I don't get why we have just 3 properties, which are configured only dynamically. Why not also dual-mode (i.e., static || dynamic)? Like for instance num.io.threads works fine in dual-mode but probably I should read a KIP, which introduced those properties... but it's bit weird.
At least I think we should add comment here that in such line DynamicConfig.java#L36 those configs are dynamic-only and not dual-mode as others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get why we have just 3 properties, which are configured only dynamically. Why not also dual-mode (i.e., static || dynamic)? Like for instance num.io.threads works fine in dual-mode but probably I should read a KIP, which introduced those properties... but it's bit weird.
You might want to check out the KIP-1051, which address the exact issue
At least I think we should add comment here that in such line DynamicConfig.java#L36 those configs are dynamic-only and not dual-mode as others.
Actually, the comment is already there. see https://github.com/apache/kafka/blob/trunk/server/src/main/java/org/apache/kafka/server/config/DynamicConfig.java#L27
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to check out the KIP-1051, which address the exact issue
So it's still under-discussion? I am bit a lost in the JIRAs like there is always at least one JIRA for issue I found :D . Because that KIP just wants to move such configuration from DynamicConfig to KafkaConfig which is not a case.
Actually, the comment is already there. see https://github.com/apache/kafka/blob/trunk/server/src/main/java/org/apache/kafka/server/config/DynamicConfig.java#L27
Class used to hold dynamic configs. These are configs which have no physical manifestation in the server.properties and can only be set dynamically.
used to (past)? It's still holding dynamic-only configs (i.e., QuotaConfig.brokerQuotaConfigs();)? And then you have also dual-mode configs which are filtered from the AbstractKafkaConfig i.e., :
// Filter and define all dynamic configurations
AbstractKafkaConfig.CONFIG_DEF.configKeys().forEach((name, value) -> {
if (DynamicBrokerConfig.ALL_DYNAMIC_CONFIGS.contains(name)) {
configs.define(value);
}
});I would probably fix that doc something like:
Holds dynamic configs, including both dynamic-only configs (e.g., quotas)
and dual-mode configs that can be set statically or dynamically.
I think it would help with overall understanding.
server/src/main/java/org/apache/kafka/server/config/AbstractKafkaConfig.java
Outdated
Show resolved
Hide resolved
|
@see-quick could you refine the title? |
Signed-off-by: see-quick <maros.orsak159@gmail.com> # Conflicts: # server/src/main/java/org/apache/kafka/server/config/DynamicBrokerConfig.java
Signed-off-by: see-quick <maros.orsak159@gmail.com>
… + refine Signed-off-by: see-quick <maros.orsak159@gmail.com>
6ac0660 to
36830ea
Compare
Signed-off-by: see-quick <maros.orsak159@gmail.com>
Signed-off-by: see-quick <maros.orsak159@gmail.com>
| ReplicationConfigs.FOLLOWER_FETCH_LAST_TIERED_OFFSET_ENABLE_CONFIG); | ||
| } | ||
|
|
||
| public static class DynamicQuotaConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a kind of decoration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it make code more readable at least for me. Like I didn't want to re-write all these properties into Dynamic<class-name>.java:
public static final Set<String> ALL_DYNAMIC_CONFIGS = Stream.of(
DYNAMIC_SECURITY_CONFIGS,
LogCleaner.RECONFIGURABLE_CONFIGS,
DynamicLogConfig.RECONFIGURABLE_CONFIGS,
DynamicThreadPool.RECONFIGURABLE_CONFIGS,
List.of(MetricConfigs.METRIC_REPORTER_CLASSES_CONFIG),
DynamicListenerConfig.RECONFIGURABLE_CONFIGS,
SocketServer.RECONFIGURABLE_CONFIGS,
DYNAMIC_PRODUCER_STATE_MANAGER_CONFIGS,
DynamicRemoteLogConfig.RECONFIGURABLE_CONFIGS,
DynamicReplicationConfig.RECONFIGURABLE_CONFIGS,
List.of(AbstractConfig.CONFIG_PROVIDERS_CONFIG),
GroupCoordinatorConfig.RECONFIGURABLE_CONFIGS,
DynamicQuotaConfig.RECONFIGURABLE_CONFIGS,
ShareCoordinatorConfig.RECONFIGURABLE_CONFIGS)
.flatMap(Collection::stream)
.collect(Collectors.toUnmodifiableSet());because you can see that some of them using that pattern DynamicThreadPool.RECONFIGURABLE_CONFIGS or DynamicLogConfig.RECONFIGURABLE_CONFIGS. Like for instance when seeing QuotaConfig.BROKER_QUOTA_CONFIGS it doesn't give me a feeling that it's dynamic configuration I have to go deep into the code to see it actually.... (maybe I am just too picky sorry :D)
| def main(args: Array[String]): Unit = { | ||
| System.out.println(configDef.toHtml(4, (config: String) => "brokerconfigs_" + config, | ||
| val combined = new ConfigDef(configDef) | ||
| QuotaConfig.brokerQuotaConfigs().configKeys().forEach((_, v) => combined.define(v)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you mind adding comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that would be good idea, I have added comment there.
Signed-off-by: see-quick <maros.orsak159@gmail.com>
This PR fixes a problem with a few broker properties. More can be found https://issues.apache.org/jira/browse/KAFKA-20110.